Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connection pool & round robin strategy #661

Merged
merged 5 commits into from
Aug 12, 2014
Merged

Conversation

chabior
Copy link

@chabior chabior commented Aug 6, 2014

Connection pool and some simple strategies with tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.16%) when pulling 384e930 on chabior:master into 92155a3 on ruflin:master.

*
* @var array
*/
protected $connections;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the $_ convention for all protected functions.

@ruflin
Copy link
Owner

ruflin commented Aug 7, 2014

This is a very nice and long expected addition (also see #357 and #333).

Please adjust the code to the coding standards (see my comments). For the tests I would suggest some more extensive tests on also "real" failing connections or also adding connection to IP's which don't exist and see what happens.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.26%) when pulling 1a810c4 on chabior:master into 92155a3 on ruflin:master.

@chabior
Copy link
Author

chabior commented Aug 7, 2014

Fixed

ruflin added a commit that referenced this pull request Aug 12, 2014
Connection pool & round robin strategy
@ruflin ruflin merged commit 07214ca into ruflin:master Aug 12, 2014
@ruflin
Copy link
Owner

ruflin commented Aug 12, 2014

Merged. Thanks a lot for this addition. I think this was long awaited :-)

@ruflin
Copy link
Owner

ruflin commented Aug 12, 2014

As far as I can see, most of this pull request is covered by your contribution: https://github.com/ruflin/Elastica/pull/357/files Can I close this one or is anything missing?

@chabior
Copy link
Author

chabior commented Aug 19, 2014

I think you can close it.

@ruflin
Copy link
Owner

ruflin commented Aug 20, 2014

Ok, will close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants